Closed
Bug 1215610
Opened 10 years ago
Closed 10 years ago
Conditional jump or move depends on uninitialised value(s) in GenerateSetSlot
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
INVALID
Tracking | Status | |
---|---|---|
firefox44 | --- | affected |
People
(Reporter: bc, Unassigned)
Details
(Keywords: sec-high, valgrind)
Attachments
(2 files)
I did a partial run of jstestbrowser under valgrind for a debug build on Fedora 22. I stopped it after a few hours but found these messages fairly early. If you find this helpful, I'll complete the run.
http://hg.mozilla.org/mozilla-central/rev/e193b4da0a8c1025aa76a403c64663ff1cd41709
vncserver :1
vncviewer :1
(DISPLAY=:1.0 make -C firefox-debug jstestbrowser EXTRA_TEST_ARGS=' --timeout=86400 --debugger=/usr/local/bin/valgrind --debugger-args=" --track-origins=yes --fair-sched=yes --smc-check=all-non-file --soname-synonyms=somalloc=NONE --vex-iropt-register-updates=allregs-at-mem-access --partial-loads-ok=yes --error-limit=no --trace-children=yes --child-silent-after-fork=yes --trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel,*/bin/uname,*/bin/which,*/bin/ps,*/bin/grep,*/bin/java --tool=memcheck --stats=yes --read-inline-info=yes --fullpath-after=/mozilla/builds/nightly/mozilla"' ) 2>&1 | tee jsreftest-valgrind.2015-10-16.txt
I see several of these messages. (See attachment)
==9316== Conditional jump or move depends on uninitialised value(s)
==9316== at 0xAAD77B6: GenerateSetSlot (/js/src/jit/IonCaches.cpp:2276)
...
==9316== Uninitialised value was created by a stack allocation
==9316== at 0xAAD9D97: js::jit::SetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, unsigned long, JS::Handle<JSObject*>, JS::Handle<JS::Value>) (/js/src/jit/IonCaches.cpp:3415)
// Guard that the incoming value is in the type set for the property
// if a type barrier is required.
if (needsTypeBarrier) {
// We can't do anything that would change the HeapTypeSet, so
// just guard that it's already there.
==> if (checkTypeset) {
masm.push(object);
CheckTypeSetForWrite(masm, obj, shape->propid(), object, value, &failurePopObject);
masm.pop(object);
}
}
bool
SetPropertyIC::update(JSContext* cx, HandleScript outerScript, size_t cacheIndex, HandleObject obj,
HandleValue value)
==>{
The following does not initialize checkTypeset
bool checkTypeset;
canCache = CanAttachNativeSetProp(cx, obj, id, cache.value(), cache.needsTypeBarrier(),
&holder, &shape, &checkTypeset);
if (!addedSetterStub && canCache == CanAttachSetSlot) {
RootedNativeObject nobj(cx, &obj->as<NativeObject>());
if (!cache.attachSetSlot(cx, outerScript, ion, nobj, shape, checkTypeset))
return false;
addedSetterStub = true;
}
Updated•10 years ago
|
Group: core-security → javascript-core-security
Comment 1•10 years ago
|
||
Hm I'm pretty sure this is a false positive. GCC is likely optimizing the code to calculate (needsTypeBarrier & checkTypeset) and branch on the result of that.
checkTypeset is set whenever needsTypeBarrier is true so this optimization is fine (if needsTypeBarrier is false, we won't hit this branch no matter what value checkTypeset has), but Valgrind doesn't know that.
NI myself to double check. We can probably refactor the code a bit to be clearer and avoid this issue.
Flags: needinfo?(jdemooij)
Comment 2•10 years ago
|
||
I cannot reproduce this even with a full run (all 6995 tests).
This is with a build by gcc-4.9.2, optimisation flags "-Og -g".
What flags are you optimising with?
Reporter | ||
Comment 3•10 years ago
|
||
-g -O
![]() |
||
Comment 4•10 years ago
|
||
> checkTypeset is set whenever needsTypeBarrier is true
Are you sure about that? Valgrind is saying that checkTypeset is not initialized when needsTypeBarrier is true.
Comment 5•10 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #4)
This might be one of those
"a && b" --> "b && a iff a is provably false when b is undefined"
transformations. Mostly they seem to be a problem only at -O2
though, so I am surprised Bob can reproduce it a -O.
Until such time as someone can reproduce this, it's all just
speculation. I'll try with -g -O.
Comment 6•10 years ago
|
||
I can reproduce this with -g -O. I suspect it's a false positive, but
some peering at the machine code didn't 100% convince me.
Is there a way to re-run just this test (jsreftest/tests/jsreftest.html?test=ecma/Array/15.4.5.2-1.js)
instead of the whole suite? That takes about 2 hours. I tried various
combinations of TEST_PATH= and just adding the path, but to no avail.
Flags: needinfo?(bob)
Reporter | ||
Comment 7•10 years ago
|
||
Unfortunately no. The test selection appears to be completely broken and the ability to run the tests individually in the browser is prevented by the use of SpecialPowers which will force a crash if run outside of the automation scripts.
Flags: needinfo?(bob)
Comment 8•10 years ago
|
||
(In reply to Bob Clary [:bc:] from comment #7)
> Unfortunately no. The test selection appears to be completely broken and the
> ability to run the tests individually in the browser is prevented by the use
> of SpecialPowers which will force a crash if run outside of the automation
> scripts.
You should be able to set security.turn_off_all_security_so_that_viruses_can_take_over_this_computer to true (seriously) and get SpecialPowers to work, though please do not browser the web using that for the reason that is described in the name of the preference. ;)
Reporter | ||
Comment 9•10 years ago
|
||
mccr8: Great thanks.
I ran a test ecma/Array/15.4.5.1-2.js with the original old build 20151015053239 via
valgrind --track-origins=yes --fair-sched=yes --smc-check=all-non-file --soname-synonyms=somalloc=NONE --vex-iropt-register-updates=allregs-at-mem-access --partial-loads-ok=yes --error-limit=no --trace-children=yes --child-silent-after-fork=yes --trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel,*/bin/uname,*/bin/which,*/bin/ps,*/bin/grep,*/bin/java --tool=memcheck --stats=yes --read-inline-info=yes --fullpath-after=/mozilla/builds/nightly/mozilla ./firefox -P jsreftest 'file:///mozilla/builds/nightly/mozilla/js/src/tests/jsreftest.html?test=ecma/Array/15.4.5.1-2.js;language=type;text/javascript' > /tmp/bug-1215610-valgrind.txt 2>&1
with a special profile initialized with the same preferences as js/src/tests/user.js though you could probably get by with just
user_pref("security.turn_off_all_security_so_that_viruses_can_take_over_this_computer", true);
user_pref("dom.max_script_run_time", 0);
user_pref("dom.max_chrome_script_run_time", 0);
I'll attach the log, but on first glance this did not reproduce the original error message but did find some others I had already seen plus perhaps a new one or two. I forgot the --show-mismatched-frees=no so it also shows the mismatched free/delete message which can be ignored.
The full test runs using the reftest harness re-use the window and running different tests so that may contain the root issue. I didn't run the other tests where the original valgrind message appeared though, so perhaps one of them would reproduce it.
Reporter | ||
Comment 10•10 years ago
|
||
log for ecma/Array/15.4.5.1-2.js
Note I packaged the specialpowers.xpi from the dist/xpi-stage directory and installed it into the special profile with the appropriate preferences set.
Reporter | ||
Comment 11•10 years ago
|
||
I wonder if mconley's experience with http://mikeconley.ca/blog/2015/10/30/a-printing-story-and-a-psa-outparams-over-the-ipc-layer-might-not-behave-like-youd-expect/ might be relevant here.
Flags: needinfo?(mconley)
Comment 12•10 years ago
|
||
(In reply to Bob Clary [:bc:] from comment #11)
> I wonder if mconley's experience with
> http://mikeconley.ca/blog/2015/10/30/a-printing-story-and-a-psa-outparams-
> over-the-ipc-layer-might-not-behave-like-youd-expect/ might be relevant here.
I appreciate the vote of confidence given my recent experience in that blog post, but from reading over this bug, I'm afraid it's a bit over my head (I've never worked with the JIT stuff). I don't think I'm going to be much help. :/
Flags: needinfo?(mconley)
Comment 13•10 years ago
|
||
Given that I can reproduce this at -g -O but not at -g -Og (which is less
aggressive in optimisation), I believe this is likely a false positive
and that we should close the bug.
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → INVALID
Updated•10 years ago
|
Flags: needinfo?(jdemooij)
Updated•10 years ago
|
Group: javascript-core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•